Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement automatic test directory for r2r #16365

Merged
merged 7 commits into from
Mar 30, 2020
Merged

Implement automatic test directory for r2r #16365

merged 7 commits into from
Mar 30, 2020

Conversation

radare
Copy link
Collaborator

@radare radare commented Mar 30, 2020

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the documentation and the radare2 book with the relevant information (if needed)

Detailed description

...

Test plan

...

Closing issues

...

@radare radare requested a review from thestr4ng3r March 30, 2020 14:10
Copy link
Contributor

@thestr4ng3r thestr4ng3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before doing any chdir based on the r2r exectuable, you must check:

  • If a test path was given manually, find the base relative to that
  • If no path was given, but there is a "db" directory in cwd, don't cd at all
    Otherwise it will be unusable in certain cases for r2ghidra for example.

binr/r2r/r2r.c Outdated Show resolved Hide resolved
binr/r2r/r2r.c Show resolved Hide resolved
binr/r2r/r2r.c Outdated Show resolved Hide resolved
binr/r2r/r2r.c Outdated Show resolved Hide resolved
@@ -94,6 +152,18 @@ int main(int argc, char **argv) {
}
}

if (r2r_dir) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to fix all paths given as args because if they are relative, they will be broken after cd'ing, making them absolute will probably be enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then we need to chdir multiple times and chdir-back to the original one after each path assuming the user can pass multiple test files from different working directories

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one run-path for all given paths is enough. But as it is right now, you will be unable to load the tests at all.

@radare
Copy link
Collaborator Author

radare commented Mar 30, 2020

pls review again

binr/r2r/r2r.c Outdated
char *cwd = NULL;
while (true) {
cwd = r_sys_getdir ();
if (r_file_is_root (cwd)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't check for root at all. If chdir fails for whatever reason you will still loop forever. Just do this instead:

char *last_cwd = NULL;

while (true) {
    last_cwd = cwd;
    cwd = r_sys_getdir();
    if (last_cwd && !strcmp (last_cwd, cwd)) {
        free (last_cwd);
        free (cwd);
        break;
    }
    free (last_cwd);

....

@radare
Copy link
Collaborator Author

radare commented Mar 30, 2020

done

@thestr4ng3r
Copy link
Contributor

The relative path issue is still not fixed, other than that lgtm.

@radare
Copy link
Collaborator Author

radare commented Mar 30, 2020

which relative path issue? passing a test in relative works fine

@thestr4ng3r
Copy link
Contributor

Screenshot from 2020-03-30 20-01-34
It takes the relative path strings given to load the database. But after chdir'ing, you are somewhere else and the relative paths don't work anymore.

@radare
Copy link
Collaborator Author

radare commented Mar 30, 2020

Ah ok got it didnt tried with directories

@radare
Copy link
Collaborator Author

radare commented Mar 30, 2020

fixed:

pmb:~ pancake$ r2r prg/radare2/test/new/db/cmd/cmd_env
Running from /Users/pancake/prg/radare2/test/new
[3/3]                       3 OK         0 BR        0 XX        0 FX
Finished in 0 seconds.
pmb:~ pancake$ r2r $PWD/prg/radare2/test/new/db/cmd/cmd_env
Running from /Users/pancake/prg/radare2/test/new
[3/3]                       3 OK         0 BR        0 XX        0 FX
Finished in 0 seconds.
pmb:~ pancake$

@radare radare merged commit 202269c into master Mar 30, 2020
@ret2libc ret2libc deleted the r2r-chdir branch September 2, 2020 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants